Add Privilege for Updating Maintenance Properties in Catalog #457
Conversation
flyrain
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
flyrain
changed the title
| TABLE_WRITE_MAINTENANCE_PROPERTIES(70, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.TABLE), | ||
| NAMESPACE_WRITE_MAINTENANCE_PROPERTIES(71, PolarisEntityType.NAMESPACE), |
There was a problem hiding this comment.
This PR focus on the catalog privilege only. The detailed implementation for table and namespace will be in follow-up PRs.
| COMPACTION(MAINTENANCE_PREFIX + "compaction"), | ||
| SNAPSHOT_RETENTION(MAINTENANCE_PREFIX + "snapshot_expiration"); |
There was a problem hiding this comment.
Will add more types, and validation in follow-up PRs.
flyrain
force-pushed
the
tms-metadata
branch
from
3b1d415
to
3351ab8
Compare
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrivilege.java
Show resolved
Hide resolved
| @@ -121,6 +122,7 @@ public enum PolarisAuthorizableOperation { | |||
| CREATE_CATALOG(CATALOG_CREATE), | |||
| GET_CATALOG(CATALOG_READ_PROPERTIES), | |||
| UPDATE_CATALOG(CATALOG_WRITE_PROPERTIES), | |||
| UPDATE_CATALOG_MAINTENANCE_PROPERTIES(CATALOG_WRITE_MAINTENANCE_PROPERTIES), | |||
There was a problem hiding this comment.
tangential: This change LGTM, but I'm thinking about breaking down the operation enum into several enums with dedicated scope (operations within a catalog, catalog management (add/remote), permission management)... Just FYI :)
flyrain
changed the title
|
|
||
| public enum PolarisMaintenanceProperties { | ||
| COMPACTION(MAINTENANCE_PREFIX + "compaction"), | ||
| SNAPSHOT_RETENTION(MAINTENANCE_PREFIX + "snapshot_expiration"); |
There was a problem hiding this comment.
I think you are referring to the "expire snapshots"? Shouldn't the enum be EXPIRE_SNAPSHOTS to align with iceberg interpretation -> https://iceberg.apache.org/docs/1.4.1/maintenance/#expire-snapshots
There was a problem hiding this comment.
Also, the enum name says RETENTION but the property has expiration – would be good to standardize.
|
|
||
| public enum PolarisMaintenanceProperties { | ||
| COMPACTION(MAINTENANCE_PREFIX + "compaction"), | ||
| SNAPSHOT_RETENTION(MAINTENANCE_PREFIX + "snapshot_expiration"); |
There was a problem hiding this comment.
Also, the enum name says RETENTION but the property has expiration – would be good to standardize.
| COMPACTION(MAINTENANCE_PREFIX + "compaction"), | ||
| SNAPSHOT_RETENTION(MAINTENANCE_PREFIX + "snapshot_expiration"); | ||
|
|
||
| private final String value; |
There was a problem hiding this comment.
the name value is misleading, it's rather the property name.
| return value; | ||
| } | ||
|
|
||
| public static PolarisMaintenanceProperties fromValue(String value) { |
There was a problem hiding this comment.
What is the added value of this method, as opposed to PolarisMaintenanceProperties.valueOf()?
Description
This is the first PRs to add table maintenance properties in Polaris. This PRs focus on adding a privilege for catalog write maintenance. Please see more details in this design doc, https://docs.google.com/document/d/1Pd_mzZcfvnUvcH98IbwsIYf4eryet1lQDfclKYx-t-M/edit?usp=sharing
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added unit test
Checklist:
Please delete options that are not relevant.